attestation crate: Add builder pattern for constructing AttestationVerifier - #70
Conversation
2722622 to
ea6b96b
Compare
ea6b96b to
d64d3d8
Compare
923b734 to
ddfdb7b
Compare
ddfdb7b to
ecfa3b7
Compare
ecfa3b7 to
6dc3123
Compare
samlaf
left a comment
samlaf
left a comment
There was a problem hiding this comment.
Leaving comments, as requested in #84 (comment)
| /// Keep a DCAP collateral cache, and pre-fill it with all available | ||
| /// collateral | ||
| pub fn with_pccs_prewarmed(mut self) -> Self { | ||
| self.internal_pccs_prewarm = Some(true); | ||
| self | ||
| } | ||
|
|
||
| /// Keep a DCAP collateral cache, starting empty | ||
| pub fn with_pccs_not_prewarmed(mut self) -> Self { | ||
| self.internal_pccs_prewarm = Some(false); | ||
| self | ||
| } | ||
|
|
||
| /// Set the URL used by internal PCCS | ||
| pub fn pccs_url(mut self, pccs_url: String) -> Self { | ||
| self.pccs_url = Some(pccs_url); | ||
| self | ||
| } | ||
| } |
There was a problem hiding this comment.
these pccs functions are a bit confusing. Didn't understand the difference between internal_pccs_prewarm = None and Some(false) at first... and then having pccs_url and internal_pccs_prewarm separate is kind of weird and even allows a combination that is nonsense (passing a url but setting prewarm to None).
How about refactoring into an enum instead?
/// How the verifier obtains DCAP collateral
#[derive(Clone, Debug)]
pub enum PccsMode {
/// No internal collateral cache. DCAP/Azure verification returns
/// [AttestationError::NoPccs].
None,
/// Internal cache pre-filled with all available collateral at build
/// time. `url` defaults to Intel PCS.
Prewarmed { url: Option<String> },
/// Internal cache that starts empty and fetches on demand.
/// `url` defaults to Intel PCS.
Lazy { url: Option<String> },
}
then you can do something like:
AttestationVerifier::builder(policy)
.with_pccs(PccsMode::Prewarmed { url: cli.pccs_url })
.with_dump_dcap_quotes(cli.log_dcap_quote)
.build()
There was a problem hiding this comment.
Yep, agree this is better. Did this, but did not put the url in because we also need the URL in the 'none' case. In the async version of the verifier, no pccs mean it will always use the remote service (given url or intel pcs) on every verification.
There was a problem hiding this comment.
Oh nice! Think this is starting to make more sense to me. Then I'd consider renaming PccsMode to CollateralCache or PccsCache something. CollateralCache::None is a lot more explicit about what its doing.
Also looks like there's a bug since in the None case the pccs_url is not being used and it always defaults to intel.
There was a problem hiding this comment.
Also found a related issue while reviewing this, created #87
There was a problem hiding this comment.
Also looks like there's a bug since in the None case the pccs_url is not being used and it always defaults to intel.
Good catch. I ended up doing quite a big refactor to address this - see: Also looks like there's a bug since in the None case the pccs_url is not being used and it always defaults to intel.
…ts/attested-tls into peg/attestation-verifier-builder * 'peg/attestation-verifier-builder' of github.com:flashbots/attested-tls: Add builder pattern for constructing AttestationVerifier Add builder pattern for constructing AttestationVerifier Run clippy on stable in CI to avoid issue with it failing with dependencies
There was a problem hiding this comment.
LGTM outside of the fix below and the ongoing discussion above
Co-authored-by: Samuel Laferriere <9342524+samlaf@users.noreply.github.com>
2 participants
This is a library API breaking change.
This adds a builder pattern for constructing AttestationVerifier. It removes the old constructor and makes several fields private which were public before, so it breaks the API.